Skip to content

deps: V8: backport --perf-prof for macOS #58010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

deps: V8: backport --perf-prof for macOS #58010

wants to merge 1 commit into from

Conversation

tmm1
Copy link

@tmm1 tmm1 commented Apr 24, 2025 •
edited by RaisinTen
Loading

@YoungWenMing @tmm1
deps: V8: backport e5dbbbadcbff
f528a3e 
Original commit message:

    Enable --perf-prof flag on MacOS

    MacOS actually can share the implementation of {PerfJitLogger} with
    Linux. From the issue 40112126, only Fuzzer tests on Windows ran
    into UNREACHABLE/FATAL because of the unimplemented {PerfJitLogger}.
    This CL enables the flag --perf-prof on MacOS.

    Bug: 403765219
    Change-Id: I97871fbcc0cb9890c51ca14fd7a6e65bd0e3c0d2
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6385655
    Reviewed-by: Clemens Backes <[email protected]>
    Reviewed-by: Matthias Liedtke <[email protected]>
    Auto-Submit: 杨文明 <[email protected]>
    Commit-Queue: Clemens Backes <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#99885}

Refs: v8/v8@e5dbbba
Co-authored-by: Aman Karmani <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Apr 24, 2025
@anonrig
Copy link
Member

anonrig commented Apr 25, 2025

Which commit does this backport? Can you share it?

@tmm1
Copy link
Author

tmm1 commented Apr 25, 2025

v8/v8@e5dbbba, via https://chromium-review.googlesource.com/c/v8/v8/+/6385655 to fix https://issues.chromium.org/issues/403765219

@tmm1
Copy link
Author

tmm1 commented Apr 25, 2025

With this change, it becomes possible to profile nodejs apps on macOS and see a full-stack profile which shows all this information in one place:

  • libSystem and other OS-level calls
  • v8 c++ internals, including garbage collection and worker threads
  • nodejs c++ internals
  • nodejs builtin javascript internals
  • c/c++/rust running in any node bindings
  • application-level javascript (both jit compiled and interpreted)

As noted in nodejs/diagnostics#652, this kind of combined profile is invaluable for investigating and fixing performance issues. Since many developers use macOS, this is especially critical to improve the performance of nodejs itself and its surrounding development tooling.

An example of how to use this patched nodejs with samply:

$ cargo install samply
$ export NODE_OPTIONS="--perf-basic-prof --perf-prof-unwinding-info --interpreted-frames-native-stack"
$ samply record node ...

For reference, other competing runtimes already offer this level of integration. For example, bun is based on JavaScriptCore which enables this feature by default. The equivalent command for them is:

$ export BUN_JSC_logJITCodeForPerf=1
$ samply record bun ...

juanarbol
juanarbol approved these changes Apr 28, 2025
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tmm1
Copy link
Author

tmm1 commented Apr 30, 2025

👋 could someone add the labels needed to move this forward? thx

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tmm1
Copy link
Author

tmm1 commented May 1, 2025

CI appears to be done. I can't tell if there are real failures I need to address.

RaisinTen
RaisinTen approved these changes May 2, 2025
View reviewed changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@targos could you please take a look when you get some time?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66539/

@targos
Copy link
Member

targos commented May 2, 2025

This commit is included in #58070.
Please don't land this on main, or it will block v24.0.0 more.

@RaisinTen
Copy link
Member

Okay, will close this in favor of your V8 upgrade PR.

@targos would it be fine if we create a PR to backport just this cherrypick to the v22 LTS branch?

@RaisinTen RaisinTen closed this May 2, 2025
@targos
Copy link
Member

targos commented May 2, 2025

Sure, as long as the V8 version in v22 is ready for it.

@RaisinTen RaisinTen mentioned this pull request May 2, 2025
Merged
@RaisinTen
Copy link
Member

Backport for LTS submitted in #58120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

@anonrig anonrig anonrig approved these changes

@juanarbol juanarbol juanarbol approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

consider reintroduce perf-prof support for macOS
9 participants
@tmm1 @nodejs-github-bot @anonrig @targos @RaisinTen @Qard @juanarbol @RafaelGSS @YoungWenMing